-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add to arrow convert #15839
feat: add to arrow convert #15839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
struct DefaultArrowConvert; | ||
impl ToArrowArrayConvert for DefaultArrowConvert {} | ||
static DEFAULT_ARROW_CONVERT: DefaultArrowConvert = DefaultArrowConvert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static variable is unnecessary since the struct is zero-size and can be instantiated at any time.
struct DefaultArrowConvert; | |
impl ToArrowArrayConvert for DefaultArrowConvert {} | |
static DEFAULT_ARROW_CONVERT: DefaultArrowConvert = DefaultArrowConvert; | |
struct DefaultArrowConvert; | |
impl ToArrowArrayConvert for DefaultArrowConvert {} |
_data_type: &arrow_schema::DataType, | ||
array: &DecimalArray, | ||
) -> Result<arrow_array::ArrayRef, ArrayError> { | ||
Ok(Arc::new(arrow_array::LargeBinaryArray::from(array))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to indicate this is special.
Decimal values are stored as ASCII text representation in a large binary array.
_data_type: &arrow_schema::DataType, | ||
array: &JsonbArray, | ||
) -> Result<arrow_array::ArrayRef, ArrayError> { | ||
Ok(Arc::new(arrow_array::LargeStringArray::from(array))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to indicate this is special.
JSON values are stored as text representation in a large string array.
#[inline] | ||
fn int16_to_arrow( | ||
&self, | ||
_data_type: &arrow_schema::DataType, | ||
array: &I16Array, | ||
) -> Result<arrow_array::ArrayRef, ArrayError> { | ||
Ok(Arc::new(arrow_array::Int16Array::from(array))) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we provide an arrow data type but we don't use it at all in the default implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's not needed in the original default implementation. But it may need for some external system, e.g. in the iceberg, we need to know the Decimal type when we want to convert a decimal array in rw to a decimal array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
ToArrow version of #15820
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.